Add archive training day feature with combined ranking #91
Conversation
This commit implements the archive training day feature which allows: - Archiving a training day by extracting attendance and ranking data - Storing attendance data (status, location, delay time, delay reasons) - Storing ranking data (tag, task scores, score history) - Deleting the contest and participations after archiving - Displaying archived training days separately from active ones - New Attendance tab to view attendance data for all archived training days New database models: - ArchivedAttendance: stores attendance data for archived training days - ArchivedStudentRanking: stores ranking data for archived training days Changes to TrainingDay model: - Added name and description fields (synced with contest while it exists) - Made contest_id nullable (NULL after archiving) New handlers: - ArchiveTrainingDayHandler: handles archiving with IP selection for location - TrainingProgramAttendanceHandler: displays attendance data with filtering UI updates: - Training days page now shows active and archived training days separately - Archive button added to active training days - New Attendance tab in training program sidebar - Archive confirmation page with class IP selection Co-Authored-By: Ron Ryvchin <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds archival support: two new ORM models (ArchivedAttendance, ArchivedStudentRanking), TrainingDay schema/relationships updates, admin handlers/templates for archiving and viewing attendance/rankings, CSS for views, and a DB migration creating archival tables and training_days columns plus FK change. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin UI
participant Handler as ArchiveTrainingDayHandler
participant DB as Database
participant Notify as Notification System
Admin->>Handler: GET archive page (show shared IPs)
Admin->>Handler: POST archive request (selected IPs, confirm)
Handler->>DB: Load TrainingDay & Contest
Handler->>DB: For each participation -> create ArchivedAttendance records
Handler->>DB: For each student -> create ArchivedStudentRanking records
Handler->>DB: Update TrainingDay archival metadata (name/description/start_time/archived_tasks_data/duration)
Handler->>DB: Delete Contest (cascades participations/submissions)
alt success
Handler->>Notify: emit success
Notify->>Admin: show confirmation
else failure
Handler->>Notify: emit error
Notify->>Admin: show error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
- Changed ArchivedStudentRanking.student_tags from JSONB to ARRAY(Unicode) - Added GIN index for efficient filtering by tag - Updated archiving logic to store all tags as a list Co-Authored-By: Ron Ryvchin <[email protected]>
The SQLAlchemy model uses Python-side default=list, not a database-level DEFAULT, so the SQL migration should not have a DEFAULT clause. Co-Authored-By: Ron Ryvchin <[email protected]>
…n.ip The starting_ip_addresses field stores the actual IPs from which students participated, while participation.ip is for allowed IP restrictions. Co-Authored-By: Ron Ryvchin <[email protected]>
- Fix 'None' has no attribute 'id' error on training_program_tasks.html by checking if training_day.contest is not none before accessing .id - Fix attendance archiving bug: lookup student by user_id instead of participation_id since Student.participation_id points to managing contest participation, not training day participation - Add start_time field to TrainingDay model to store contest start time after archiving - Display start_time in archived training days table with sorting Co-Authored-By: Ron Ryvchin <[email protected]>
- Fix ScoreHistory.time -> ScoreHistory.timestamp in archiving code (the model uses 'timestamp' not 'time') - Fix sidebar to filter out archived training days (where contest is None) to prevent 'None' has no attribute errors when accessing td.contest Co-Authored-By: Ron Ryvchin <[email protected]>
- Add colored badges with icons for student statuses (On Time, Delayed, Missed) - Add colored badges for location (Class, Home, Both) - Color columns within each training day (status, location, reasons) - Change training day titles to show description with start date in brackets - Fix date filtering to actually filter archived training days by start_time - Order results by start_time instead of position - Add Clear button to reset date filters - Fix location to default to 'home' when no class IP is chosen during archiving - Fix location to default to 'home' when participant has no IP recorded Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cms/server/admin/templates/training_program_attendance.html`:
- Around line 233-239: The template builds the student edit link with the wrong
ID: it uses student.participation.user.id but the route expects the student ID;
update the link generation to pass student.id (i.e. change the url call in the
table header from student.participation.user.id to student.id so the path
url('training_program', training_program.id, 'student', student.id, 'edit')
points to the correct handler).
🧹 Nitpick comments (6)
cms/server/admin/templates/archive_training_day.html (1)
58-66: Consider adding explicit confirmation for this destructive action.Since archiving permanently deletes the contest and participations, consider adding an additional confirmation mechanism such as requiring the admin to type the contest name or check a confirmation checkbox. This provides an extra safeguard against accidental archiving.
💡 Optional: Add confirmation checkbox
<h2>Confirm Archive</h2> <p> + <label> + <input type="checkbox" name="confirm_archive" required /> + I understand this action is irreversible and will permanently delete the contest + </label> + </p> + <p> <input type="submit" value="Archive Training Day" {% if not admin.permission_all %} disabled {% endif %} /> </p>cms/server/admin/templates/training_program_attendance.html (1)
259-269: Consider adding a fallback for missing location.When
att.locationisNone, no location is displayed. While the archiving handler should always set a value, adding a fallback would improve robustness.Optional: Add fallback for None location
<div class="location-row"> {% if att.location == "class" %} <span class="location-icon">🏫</span> Class {% elif att.location == "home" %} <span class="location-icon">🏠</span> Home {% elif att.location == "both" %} <span class="location-icon">⇄</span> Both {% elif att.location %} <span>{{ att.location }}</span> + {% else %} + <span class="location-icon">❓</span> Unknown {% endif %} </div>cms/db/training_day.py (1)
123-126: Type annotation inconsistent with nullable contest_id.The
contestrelationship type hint is"Contest"but sincecontest_idis now nullable, the relationship can beNoneafter archiving. Consider updating the type annotation for consistency.Suggested fix
- contest: "Contest" = relationship( + contest: "Contest | None" = relationship( "Contest", back_populates="training_day", )cms/server/admin/handlers/trainingprogram.py (3)
2175-2175: Consider moving import to top of file.The
DelayRequestimport is inside the method. While this works, it's more idiomatic to import at the module level for consistency with other imports.Suggested change at top of file (around line 30-33)
from cms.db import Contest, TrainingProgram, Participation, Submission, \ User, Task, Question, Announcement, Student, StudentTask, Team, \ TrainingDay, TrainingDayGroup, ArchivedAttendance, ArchivedStudentRanking, \ - ScoreHistory + ScoreHistory, DelayRequestThen remove the import from line 2175.
2228-2228: Consider moving import to top of file.Similar to
DelayRequest,ParticipationTaskScoreis imported inside the method. Consider adding it to the module-level imports for consistency.Suggested change at top of file (around line 30-33)
from cms.db import Contest, TrainingProgram, Participation, Submission, \ User, Task, Question, Announcement, Student, StudentTask, Team, \ TrainingDay, TrainingDayGroup, ArchivedAttendance, ArchivedStudentRanking, \ - ScoreHistory + ScoreHistory, DelayRequest, ParticipationTaskScoreThen remove the import from line 2228.
2305-2306: Use existing datetime import for timedelta.The
timedeltais imported inside the method, butdatetimeis already imported asdtat the top. You can usedtmodule's timedelta or import timedelta alongside at module level.Suggested fix
Change line 24 to include timedelta:
-from datetime import datetime as dt +from datetime import datetime as dt, timedeltaThen change lines 2305-2306 to:
- from datetime import timedelta query = query.filter(TrainingDay.start_time < end_date + timedelta(days=1))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cms/db/__init__.pycms/db/archived_attendance.pycms/db/archived_student_ranking.pycms/db/training_day.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/templates/archive_training_day.htmlcms/server/admin/templates/base.htmlcms/server/admin/templates/training_program_attendance.htmlcms/server/admin/templates/training_program_tasks.htmlcms/server/admin/templates/training_program_training_days.htmlcmscontrib/updaters/update_from_1.5.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-09T19:33:59.218Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cms/server/admin/handlers/dataset.py:1323-1387
Timestamp: 2026-01-09T19:33:59.218Z
Learning: In cms/server/admin/handlers/**/*.py, follow the existing pattern: do not emit explicit error notifications when try_commit() fails. Rely on centralized error handling and logging instead. Apply this consistently to all new and updated handlers to maintain uniform behavior and maintainability.
Applied to files:
cms/server/admin/handlers/trainingprogram.pycms/server/admin/handlers/__init__.py
🧬 Code graph analysis (4)
cms/db/archived_attendance.py (3)
cms/db/base.py (1)
Base(67-323)cms/db/training_day.py (1)
TrainingDay(65-156)cms/db/student.py (1)
Student(38-89)
cms/db/__init__.py (2)
cms/db/archived_attendance.py (1)
ArchivedAttendance(38-98)cms/db/archived_student_ranking.py (1)
ArchivedStudentRanking(38-97)
cms/db/training_day.py (2)
cms/db/archived_attendance.py (1)
ArchivedAttendance(38-98)cms/db/archived_student_ranking.py (1)
ArchivedStudentRanking(38-97)
cms/server/admin/handlers/__init__.py (1)
cms/server/admin/handlers/trainingprogram.py (2)
ArchiveTrainingDayHandler(2015-2267)TrainingProgramAttendanceHandler(2270-2341)
🪛 Ruff (0.14.11)
cms/server/admin/handlers/trainingprogram.py
2098-2098: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (25)
cms/server/admin/templates/base.html (2)
313-317: LGTM! Correctly filters out archived training days from sidebar.The filter
if td.contestappropriately excludes archived training days (wherecontest_idis null) from the sidebar navigation, since they no longer have active contests to link to.
482-482: LGTM! New Attendance navigation entry.The new menu entry follows the existing pattern and provides navigation to the attendance feature for archived training days.
cms/db/__init__.py (2)
58-58: LGTM! New archived models added to exports.The new
ArchivedAttendanceandArchivedStudentRankingmodels are correctly added to__all__alongside related training program models.
114-115: LGTM! Import statements follow existing pattern.The imports for the new archived data models are consistent with the module's import organization.
cms/server/admin/templates/training_program_tasks.html (1)
76-80: LGTM! Properly handles archived training days without contests.The updated conditional logic correctly handles the case where a training day exists but its contest has been archived (deleted). The fallback to
td.nameor"(archived)"provides clear indication to users.cms/server/admin/templates/training_program_training_days.html (4)
12-13: LGTM! Clear separation of active and archived training days.The template correctly uses
selectattr("contest")to filter active training days (those with associated contests) and presents them in a dedicated section with the ability to archive.Also applies to: 48-49
63-65: LGTM! Archive action link added.The archive link correctly routes to the archive confirmation page for each active training day.
82-112: LGTM! Well-structured archived training days section.The archived section correctly uses
rejectattr("contest")to show only archived days, and provides appropriate fallbacks for missing name/description/start_time fields.
114-118: LGTM! Table sorting enabled for archived days.The sort initialization for the archived table uses correct column index (2 for Start column in the 3-column table).
cms/server/admin/templates/archive_training_day.html (2)
12-21: LGTM! Clear warning about destructive action.The warning notification clearly communicates the consequences of archiving, including the permanent deletion of the contest and participations. This is important for preventing accidental data loss.
33-56: LGTM! Well-designed class IP selection interface.The IP selection table is sorted by student count in descending order (most common IPs first), making it easier for admins to identify classroom IPs. The fallback message when no shared IPs exist is appropriate.
cms/db/archived_attendance.py (1)
38-98: LGTM! Well-structured archival model.The model correctly implements the archival pattern with:
- Appropriate FK constraints with CASCADE delete behavior
- Composite unique constraint preventing duplicate records
- Proper indexing on foreign key columns
- Clear separation between required (
status) and optional (location,delay_time,delay_reasons) fieldscms/server/admin/handlers/__init__.py (1)
187-189: LGTM! Routes and imports follow established patterns.The new handlers are properly imported and routed, consistent with the existing training program URL structure.
Also applies to: 361-362
cmscontrib/updaters/update_from_1.5.sql (1)
568-664: LGTM! Migration is well-structured and complete.The SQL migration correctly:
- Adds archival columns to
training_days(name, description, start_time)- Changes
contest_idfrom CASCADE to SET NULL for proper archival behavior- Creates both archival tables with appropriate constraints, indexes (including GIN for tags), and foreign keys matching the ORM models
cms/db/archived_student_ranking.py (1)
38-97: LGTM! Well-designed archival ranking model.The model correctly implements:
- JSONB columns for flexible storage of task scores and score history
- PostgreSQL ARRAY with GIN index for efficient tag-based filtering
- Consistent structure with
ArchivedAttendancemodel- Proper type hints reflecting the expected data structures
cms/db/training_day.py (3)
86-92: LGTM on the nullable contest_id change.The change to make
contest_idnullable withondelete="SET NULL"correctly supports the archival workflow where the contest is deleted but the training day record persists.
99-116: LGTM on the new archival fields.The
name,description, andstart_timefields appropriately preserve contest metadata after archiving. The comments clearly document the sync-while-active / preserve-after-archive behavior.
146-156: LGTM on the archived relationships.The
cascade="all, delete-orphan"configuration ensures archived attendance and ranking data are properly cleaned up when a training day is deleted.cms/server/admin/handlers/trainingprogram.py (7)
32-33: LGTM on the new imports.The imports for
ArchivedAttendance,ArchivedStudentRanking, andScoreHistoryare necessary for the archiving functionality.
449-451: LGTM on skipping archived training days.Correctly guards against attempting to create participations for archived training days that no longer have an associated contest.
2018-2056: LGTM on the archive confirmation page handler.The validation checks and IP counting logic are well-implemented. The shared IP detection (filtering to IPs with >1 student) provides useful context for the admin to identify class IPs.
2098-2103: Broad exception handling is consistent with existing patterns.While static analysis flags
except Exception, this follows the established handler pattern in this codebase for gracefully handling various failure modes during complex operations. The notification and redirect behavior is appropriate. Based on learnings, the centralized error handling aftertry_commit()is relied upon for commit failures.
2114-2196: LGTM on attendance archival logic.The attendance extraction correctly:
- Looks up students via
Participation.user_idjoin (since Student references managing contest participation)- Handles all location edge cases (no class IPs, no participant IPs, mixed IPs)
- Defaults to "home" when IP data is unavailable
- Aggregates delay reasons from DelayRequest records
2198-2267: LGTM on ranking archival logic.The ranking extraction correctly:
- Preserves all student tags as an array for filtering
- Stores task scores keyed by task ID (as string for JSON compatibility)
- Converts score history timestamps to offsets from contest start in RWS format
2270-2341: LGTM on attendance display handler.The handler correctly:
- Filters archived training days by date range
- Builds a student-to-training-day attendance matrix
- Orders training days by start_time for consistent display
- Handles missing participation gracefully in the sort key
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ronryv
force-pushed
the
devin/1768469828-archive-training-day
branch
from
a09184a to
7b6ee72
Compare
- Add TrainingProgramCombinedRankingHandler for main page with date filtering - Add TrainingProgramCombinedRankingHistoryHandler for JSON history endpoint - Add TrainingProgramCombinedRankingDetailHandler for student detail view - Create templates for combined ranking and detail pages - Add navigation link in sidebar - Support multi-contest format in history view (each training day is a contest) Co-Authored-By: Ron Ryvchin <[email protected]>
devin-ai-integration
bot
changed the title
The archiving code was incorrectly using contest.tasks (which is empty for training day contests) instead of training_day.tasks. Also changed to use the task_score function with the managing contest participation to compute actual scores, matching how the ranking handler works. Changes: - Use training_day.tasks instead of contest.tasks - Get managing contest participation for each user - Use task_score() to compute scores filtered by training day - Filter score history to only include tasks in the training day Co-Authored-By: Ron Ryvchin <[email protected]>
Changed the archiving code to use get_cached_score_entry which: 1. Uses the score cache efficiently instead of recalculating 2. Handles training day participations correctly (queries submissions from managing contest filtered by training_day_id) 3. Uses training_day.tasks instead of contest.tasks Also fixed ScoreHistory query to use training day participation ID since history is stored with the training day participation. Co-Authored-By: Ron Ryvchin <[email protected]>
…nd task metadata - Store task_scores for ALL visible tasks (including 0 scores) to differentiate between inaccessible tasks and 0-score tasks - Add submissions field to ArchivedStudentRanking for rendering submission tables - Add archived_tasks_data to TrainingDay to preserve scoring scheme at archive time - Update combined ranking table to show N/A for inaccessible tasks - Update combined ranking detail page to use archived task metadata and submissions - Simplify data model: use task_scores keys for visibility instead of separate field Co-Authored-By: Ron Ryvchin <[email protected]>
Co-Authored-By: Ron Ryvchin <[email protected]>
…tTask records When archiving a training day, determine which tasks should be visible to each student based on their tags (using can_access_task), rather than only including tasks where the student already has a StudentTask record. This ensures students who missed the start button still get the training day's tasks in their archive with 0 scores. Co-Authored-By: Ron Ryvchin <[email protected]>
When archiving a training day, create StudentTask records for visible tasks that the student doesn't already have. This allows students who missed the training to still submit those tasks from home via the training program management contest, mirroring the behavior of the start button. Co-Authored-By: Ron Ryvchin <[email protected]>
The contest begin/end times were set to absolute timestamps (td.start_time), but the history and submission times are stored as offsets from the original contest start. This caused: 1. Graph scale to be incorrect (data points outside the visible range) 2. Submission times to display incorrectly (negative values) Fix: Set contest begin to 0 and calculate end from the max history time, since all times are stored as offsets from contest start. Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cms/server/admin/static/aws_style.css`:
- Around line 1223-1292: The global selectors .page-header and .btn-primary in
aws_style.css are overriding Bootstrap site-wide; scope them by nesting or
renaming—e.g., change selectors to .training-program-view .page-header and
.training-program-view .btn-primary (or rename to .tp-page-header and
.tp-btn-primary) and update any templates that render training program pages to
wrap content in the .training-program-view container so only
training-program-specific headers and primary buttons get the new flex and teal
styles while leaving Bootstrap's defaults elsewhere; keep other classes like
.filter-card/.filter-group/.btn-text unchanged.
In `@cms/server/admin/templates/training_program_combined_ranking.html`:
- Around line 86-91: The empty-state colspan currently uses "{{ 1 +
archived_training_days|length }}" which only covers the date column and archived
training day columns; update it to compute the full number of table columns by
adding the task columns as well (e.g. replace with "{{ 1 +
archived_training_days|length + tasks|length }}" or the correct task-list
variable used in this template) so the "No students found" row spans all task
columns and keeps the table aligned.
🧹 Nitpick comments (2)
cmscontrib/updaters/update_from_1.5.sql (1)
568-572: Consider populatingnameanddescriptionfor existing rows.After adding these nullable columns, existing
training_daysrows will haveNULLvalues fornameanddescription. If the application code expects these to be populated (even for active training days with contests), you may want to add anUPDATEstatement to initialize them from the associated contest data.Proposed migration addition
-- After adding the columns, populate from existing contest data UPDATE public.training_days td SET name = c.name, description = c.description FROM public.contests c WHERE td.contest_id = c.id AND td.name IS NULL;cms/server/admin/handlers/trainingprogram.py (1)
2385-2413: Consider extracting date filtering logic.The date parsing and archived training day query pattern is duplicated across
TrainingProgramAttendanceHandler,TrainingProgramCombinedRankingHandler,TrainingProgramCombinedRankingHistoryHandler, andTrainingProgramCombinedRankingDetailHandler.A helper method could reduce duplication:
♻️ Example helper extraction
def _parse_date_range(self) -> tuple[dt | None, dt | None]: """Parse start_date and end_date query arguments.""" start_date = end_date = None start_str = self.get_argument("start_date", None) end_str = self.get_argument("end_date", None) if start_str: try: start_date = dt.fromisoformat(start_str) except ValueError: pass if end_str: try: end_date = dt.fromisoformat(end_str) except ValueError: pass return start_date, end_date def _get_archived_training_days( self, training_program_id: int, start_date: dt | None, end_date: dt | None ) -> list[TrainingDay]: """Query archived training days with optional date filtering.""" query = ( self.sql_session.query(TrainingDay) .filter(TrainingDay.training_program_id == training_program_id) .filter(TrainingDay.contest_id.is_(None)) ) if start_date: query = query.filter(TrainingDay.start_time >= start_date) if end_date: query = query.filter(TrainingDay.start_time < end_date + timedelta(days=1)) return query.order_by(TrainingDay.start_time).all()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cms/db/archived_student_ranking.pycms/db/training_day.pycms/server/admin/handlers/__init__.pycms/server/admin/handlers/trainingprogram.pycms/server/admin/static/aws_style.csscms/server/admin/templates/archive_training_day.htmlcms/server/admin/templates/base.htmlcms/server/admin/templates/student_tasks.htmlcms/server/admin/templates/training_program_attendance.htmlcms/server/admin/templates/training_program_combined_ranking.htmlcms/server/admin/templates/training_program_combined_ranking_detail.htmlcms/server/admin/templates/training_program_task_remove.htmlcms/server/contest/templates/task_submissions.htmlcmscontrib/updaters/update_from_1.5.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- cms/server/admin/templates/training_program_attendance.html
- cms/server/admin/templates/archive_training_day.html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-09T19:33:59.218Z
Learnt from: ronryv
Repo: ioi-isr/cms PR: 68
File: cms/server/admin/handlers/dataset.py:1323-1387
Timestamp: 2026-01-09T19:33:59.218Z
Learning: In cms/server/admin/handlers/**/*.py, follow the existing pattern: do not emit explicit error notifications when try_commit() fails. Rely on centralized error handling and logging instead. Apply this consistently to all new and updated handlers to maintain uniform behavior and maintainability.
Applied to files:
cms/server/admin/handlers/trainingprogram.pycms/server/admin/handlers/__init__.py
🧬 Code graph analysis (4)
cms/db/training_day.py (5)
cms/db/base.py (1)
Base(67-323)cms/db/training_program.py (1)
TrainingProgram(36-81)cms/db/training_day_group.py (1)
TrainingDayGroup(37-82)cms/db/archived_attendance.py (1)
ArchivedAttendance(38-98)cms/db/archived_student_ranking.py (1)
ArchivedStudentRanking(38-106)
cms/server/admin/handlers/trainingprogram.py (8)
cms/db/contest.py (1)
Contest(47-384)cms/db/training_program.py (1)
TrainingProgram(36-81)cms/db/training_day.py (1)
TrainingDay(66-165)cms/db/archived_attendance.py (1)
ArchivedAttendance(38-98)cms/db/archived_student_ranking.py (1)
ArchivedStudentRanking(38-106)cms/db/scorecache.py (1)
ScoreHistory(120-168)cms/server/util.py (3)
get_all_student_tags(64-79)deduplicate_preserving_order(436-451)can_access_task(157-193)cms/server/contest/handlers/trainingprogram.py (1)
get(44-167)
cms/db/archived_student_ranking.py (3)
cms/db/base.py (1)
Base(67-323)cms/db/training_day.py (1)
TrainingDay(66-165)cms/db/student.py (1)
Student(38-89)
cms/server/admin/handlers/__init__.py (1)
cms/server/admin/handlers/trainingprogram.py (6)
RemoveTrainingDayGroupHandler(1731-1749)ArchiveTrainingDayHandler(2030-2374)TrainingProgramAttendanceHandler(2377-2447)TrainingProgramCombinedRankingHandler(2450-2543)TrainingProgramCombinedRankingHistoryHandler(2546-2595)TrainingProgramCombinedRankingDetailHandler(2598-2776)
🪛 Ruff (0.14.11)
cms/server/admin/handlers/trainingprogram.py
2113-2113: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (debian:bookworm)
- GitHub Check: test (ubuntu:noble)
🔇 Additional comments (26)
cms/server/admin/static/aws_style.css (2)
1294-1382: Sticky header + first-column implementation looks solid.Good use of sticky positioning, z-index layering, and overflow handling to keep headers and student names visible.
1384-1498: Attendance & combined ranking visual styles look good.The badge system, date/task headers, and score cell differentiation are clear and consistent with the rest of the admin theme.
cms/server/admin/templates/training_program_task_remove.html (1)
11-14: LGTM!The additional
task.training_day.contestcheck correctly guards against accessing.contest.descriptionwhen the training day has been archived (contest_id is NULL). This aligns well with the nullable contest_id schema change.cms/server/admin/templates/base.html (2)
313-317: LGTM!The filter
if td.contestcorrectly excludes archived training days (wherecontest_idis NULL) from the sidebar navigation. This prevents displaying broken links and aligns with the schema change makingcontest_idnullable.
482-483: LGTM!New navigation entries for Attendance and Combined Ranking tabs are correctly placed within the training program menu section.
cmscontrib/updaters/update_from_1.5.sql (3)
582-620: LGTM!The
archived_attendancestable schema is well-designed:
- Unique constraint on
(training_day_id, student_id)prevents duplicate entries- Proper foreign keys with CASCADE delete for referential integrity
- Appropriate indexes on both foreign key columns
621-661: LGTM!The
archived_student_rankingstable is properly structured:
- GIN index on
student_tagsarray enables efficient tag-based queries- JSONB columns for flexible storage of scores, submissions, and history
- Consistent constraint and index patterns with the attendance table
663-668: LGTM!Adding
start_timeandarchived_tasks_datacolumns completes the archive metadata storage requirements for training days.cms/server/contest/templates/task_submissions.html (1)
411-411: LGTM!The fallback chain
training_day.contest.description if training_day.contest else (training_day.description or training_day.name)correctly handles archived training days where the contest has been deleted. This ensures a meaningful description is always displayed.cms/server/admin/templates/student_tasks.html (1)
57-61: LGTM!The conditional logic correctly handles archived training days:
- When
contestexists: renders a clickable link to the contest- When
contestis NULL: displays the training day's description/name with a sensible "Archived Training Day" fallbackThis prevents broken links and provides clear context for archived sources.
cms/server/admin/templates/training_program_combined_ranking.html (1)
8-22: Filter controls look good.
Clear date inputs + reset link make the flow straightforward for admins.cms/server/admin/templates/training_program_combined_ranking_detail.html (1)
317-412: JS wiring for the archived detail view looks solid.
The override/init flow cleanly adapts the existing renderer to archived data.cms/db/training_day.py (1)
87-125: Archival metadata fields align with the new workflow.
Persisting name/description/start_time/tasks data after contest deletion is consistent with the archive semantics.cms/db/archived_student_ranking.py (1)
38-105: Model definition looks consistent with the archival requirements.
Constraints and column structure are coherent for per‑student historical data.cms/server/admin/handlers/__init__.py (1)
364-368: Routes for archive/attendance/combined ranking views are wired cleanly.cms/server/admin/handlers/trainingprogram.py (11)
24-49: LGTM!Import additions are well-organized and align with the new archiving functionality. The utilities (
can_access_task,get_all_student_tags, etc.) and models (ArchivedAttendance,ArchivedStudentRanking, etc.) are appropriately imported for the new handlers.
464-466: LGTM!Good defensive check to skip archived training days (where
contestisNone) when adding a new student. This prevents attempting to create participations for contests that no longer exist.
2030-2071: LGTM!The GET handler correctly validates ownership, checks archive status, and computes shared IPs for class IP selection. The logic to identify IPs with more than one student (
count > 1) effectively filters for likely classroom IPs.
2098-2127: Archive operation is well-structured.The archiving flow correctly:
- Preserves contest metadata on the TrainingDay before deletion
- Archives attendance and ranking data
- Deletes the contest (cascading to participations)
The broad
Exceptioncatch at line 2113 (flagged by static analysis) follows the existing codebase pattern for form submission handlers. Based on learnings, this is consistent with how other handlers in this file manage errors.
2129-2210: LGTM!The attendance archival logic correctly handles:
- Student lookup via
user_id(sinceStudent.participation_idreferences the managing contest, not training day)- Location determination with proper handling of edge cases (no class IPs, no IP recorded, mixed IPs)
- Delay reason aggregation from
DelayRequestrecords
2212-2298: Well-designed ranking archival logic.Key aspects are correctly implemented:
can_access_taskdetermines per-student task visibility based on tagsStudentTaskrecords are created for visible tasks, enabling students who missed training to submit later- Task metadata is preserved in
archived_tasks_datafor historical accuracyThe inline import of
get_cached_score_entryat line 2228 is acceptable for avoiding potential circular imports.
2299-2374: LGTM!The score and submission data collection is well-implemented:
- Time offsets are calculated consistently from
contest.start- String keys for
task_scoresalign with JSONB storage patterns- Nullable handling (
if task_scores else None) avoids storing empty dicts
2377-2447: LGTM!Attendance display handler correctly:
- Filters for archived training days (
contest_id.is_(None))- Applies optional date range filtering
- Builds a nested dict structure for easy template access
- Gracefully handles students without participation records in sort key
2546-2595: LGTM!History endpoint correctly aggregates score history entries across archived training days and returns them in the expected JSON format for graph rendering.
2598-2776: Well-implemented detail view handler.The handler correctly:
- Calculates graph scale with
begin=0andendfrom max history time (fixing the submission time display per PR description)- Preserves original scoring scheme via
archived_tasks_data- Builds comprehensive data structures for the detail template
Note: The
Query.get()usage at line 2683 has the same SQLAlchemy 2.0 deprecation concern flagged above.
2512-2520: : The codebase uses SQLAlchemy 1.3.24 (constrained to<1.4), whereQuery.get()is the standard and correct pattern. Deprecation warnings forQuery.get()only apply to SQLAlchemy 2.0+. This pattern is used consistently throughout the codebase, including in utility functions indb/base.py, and requires no changes.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1. Fix submission fetching to use managing_participation.id, training_day_id, and official=True filter when archiving 2. Fix submission time offset to be relative to student start time 3. Add duration field to TrainingDay model and calculate at archiving time (max of main groups duration or training day duration) 4. Handle archived training days (where contest is None) in all places: - ScoringService: already checks contest_id is not None - Templates: show text instead of links for archived training days - Handlers: skip operations for archived training days - Submission download: use stored description/name for archived training days Co-Authored-By: Ron Ryvchin <[email protected]>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@cms/server/admin/handlers/trainingprogram.py`:
- Around line 2678-2738: The task ordering is unstable because
task_ids_in_contest is iterated as a set; change the loop that currently reads
"for task_id in task_ids_in_contest:" to iterate over a sorted list (e.g., "for
task_id in sorted(task_ids_in_contest):") so tasks/columns are produced
deterministically; update any related aggregation (contest_tasks,
contest_max_score, submissions_data population) to rely on this sorted order so
the detailed ranking view remains stable (refer to task_ids_in_contest,
contest_tasks, contest_max_score, and the loop body that builds
tasks_data/submissions_data).
- Around line 2532-2585: The UI columns can shuffle because task_scores (JSONB)
keys are unordered; in TrainingProgramCombinedRankingHandler when building
training_day_tasks from visible_tasks_by_id you should produce a deterministic
ordering — e.g., sort the tasks before assigning training_day_tasks[td.id].
Locate the block that creates visible_tasks_by_id and replace the final
assignment training_day_tasks[td.id] = list(visible_tasks_by_id.values()) with a
sorted list (stable key such as task id or name) so training_day_tasks[td.id]
always has tasks in the same order regardless of JSON key order; ensure any
other consumption of visible_tasks_by_id follows the same sorted order.
- Around line 2080-2131: Replace the broad "except Exception as error" in the
post method with a catch for SQLAlchemyError (or a more specific sqlalchemy.exc
subclass) so programming errors still propagate; import SQLAlchemyError from
sqlalchemy.exc, change the handler to "except SQLAlchemyError as error:", keep
the existing service.add_notification and redirect logic there, and do not
swallow other exceptions (remove the generic Exception handler) so non-database
errors surface to centralized error handling; reference the post method, the
sql_session.delete(contest) call, and service.add_notification when making this
change.
In `@cms/server/admin/static/aws_style.css`:
- Line 1: Remove the leading BOM character before the html selector in
aws_style.css so the selector reads "html {" without any invisible characters;
locate the start of the file (the "html" selector) and delete the BOM (UTF-8 BOM
/ U+FEFF) so the linter no longer sees an invalid selector.
In `@cms/server/contest/templates/task_submissions.html`:
- Line 411: The header's td_desc expression can end up empty when both
training_day.description and training_day.name are blank; update the td_desc
fallback chain in the trans block so it includes a final literal fallback (e.g.,
"Archived training day" or "Unnamed training day") after checking
training_day.contest.description, training_day.description, and
training_day.name so the H3 never renders blank—modify the td_desc computation
used in the <h3> (training_day, td_desc) to append that final fallback string.
♻️ Duplicate comments (3)
cms/server/admin/templates/training_program_attendance.html (1)
47-48: Use the student ID in the edit link.
The edit route expects the student ID, but the template passes the user ID. This can lead to 404s or editing the wrong record.🐛 Proposed fix
- {{ student.participation.user.first_name }} {{ student.participation.user.last_name }} (<a href="{{ url('training_program', training_program.id, 'student', student.participation.user.id, 'edit') }}">{{ student.participation.user.username }}</a>) + {{ student.participation.user.first_name }} {{ student.participation.user.last_name }} (<a href="{{ url('training_program', training_program.id, 'student', student.id, 'edit') }}">{{ student.participation.user.username }}</a>)cms/server/admin/templates/training_program_combined_ranking_detail.html (1)
200-204: Use the student ID in the edit link.
The route expects the student ID, but the template passes the user ID.🐛 Proposed fix
- <a href="{{ url("training_program", training_program.id, "student", student.participation.user.id, "edit") }}">Edit Student</a> + <a href="{{ url("training_program", training_program.id, "student", student.id, "edit") }}">Edit Student</a>cms/server/admin/templates/training_program_combined_ranking.html (1)
82-85: Use the student ID in the edit link.
The edit route expects the student ID, but the template passes the user ID.🐛 Proposed fix
- <a class="student-name-link" href="{{ url('training_program', training_program.id, 'student', student.participation.user.id, 'edit') }}"> + <a class="student-name-link" href="{{ url('training_program', training_program.id, 'student', student.id, 'edit') }}">
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-Authored-By: Ron Ryvchin <[email protected]>
Add archive training day feature with combined ranking
Summary
This PR implements the archive training day feature for training programs. When a training day is archived:
New database models
ArchivedAttendanceandArchivedStudentRankingstore the extracted data. TheTrainingDaymodel now has nullablecontest_id(SET NULL on delete) and newname/description/start_time/archived_tasks_data/durationfields.UI changes include:
Updates since last revision
training_day_numfor stable ordering. Thetraining_day_numis saved toarchived_tasks_dataduring archiving and used to sort tasks in bothTrainingProgramCombinedRankingHandlerandTrainingProgramCombinedRankingDetailHandler. Falls back to task ID for consistent ordering whentraining_day_numis not available.Previous updates (still relevant)
managing_participation.id(not training day participation), filtered bytraining_day_idandofficial=True. This ensures only official submissions made via the specific training day are archived.starting_time(if available) rather than the contest start time.durationfield to TrainingDay model, calculated as max duration among main groups (if any) or the training day duration (if no main groups).training_day.contestis accessed to handle the case where contest is None (archived):StudentTaskrecords are now created for visible tasks that the student doesn't already have.can_access_task).visible_task_idsfield fromArchivedStudentRanking. The presence of a task_id key intask_scoresnow indicates the task was visible.archived_tasks_data) is now stored once onTrainingDayinstead of duplicated per-student.ArchivedStudentRankingnow stores submission data for each task in RWS format.get_cached_score_entry()for archiving: Per feedback, changed from usingtask_score()toget_cached_score_entry().ARRAY(Unicode)with a GIN index.Review & Testing Checklist for Human
Verify task ordering is stable - Archive a training day with multiple tasks, then view the Combined Ranking tab multiple times. Verify tasks always appear in the same order (by
training_day_num).Test submission fetching during archiving - Archive a training day and verify that only official submissions made via that training day are included in the archived data (not submissions from other training days or the task archive).
Verify submission time offsets - After archiving, check the history view and verify submission times are relative to the student's start time (not contest start). Students who started late should have their submission times adjusted accordingly.
Test archived training day handling - Navigate to pages that display training day info (tasks list, submission download, training day remove page) and verify archived training days show their stored name/description instead of crashing.
Test history view graphs and submission times - Click "[history]" for a student with submissions and verify: (1) the score/rank graphs display with correct scale, (2) submission times in the table display correctly.
Recommended test plan:
visible_to_tagsrestrictions)Notes
Summary by CodeRabbit
New Features
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.